Skip to content

fix(gc): restore the census-pinned carrier expression in the shape scanner - #8918

Merged
proggeramlug merged 1 commit into
mainfrom
fix/8899-census-carrier-pin
Aug 28, 2026
Merged

fix(gc): restore the census-pinned carrier expression in the shape scanner#8918
proggeramlug merged 1 commit into
mainfrom
fix/8899-census-carrier-pin

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

lint has been red on main since #8899 landed, and stayed red across #8900 and #8901. This restores it.

What broke

#8899 lifted descriptor.old_carrier || descriptor.cache_carrier out of the if in scan_shape_table_rekey_mut into a let is_carrier = … binding, so the memo could key on it. The change is semantically inert — is_carrier is that expression.

But scripts/shape_descriptor_census.py pins the whole two-armed expression on purpose, and says so:

Pin the whole two-armed expression, not just the set of APIs called: a sabotage that widens the gate, or that swaps the arms, has to be red.

So the pin stopped matching and the census started raising CensusError: descriptor rooting is not gated on 'old_carrier || cache_carrier'.

The second-order damage

The census's own self-test sabotages that exact literal:

ungated_root[shapes_path] = ungated_root[shapes_path].replace(
    "let moved = if descriptor.old_carrier || descriptor.cache_carrier {",
    "let moved = if true {", 1,
)

str.replace does nothing when the string is absent. After #8899 that sabotage replaced nothing, so the "un-gated into an unconditional table root" case was passing without testing anything. A sabotage test that patches a string literal goes vacuous the moment the source is reworded, and nothing reports it.

The fix

The condition is written out again at the decision site, with a comment saying why it must stay literal. is_carrier remains and still keys the memo. One line of behaviour change: none.

Attribution

f9890759c exit=1   bench: dynamic property keys (#8901)
e18b24c42 exit=1   gc(shapes): transition cache weak target (#8900)
d02892491 exit=1   perf(gc): memoise the shape scanner probe (#8899)   <-- broke here
2a6dcd344 exit=0   perf: ECS command path round 3 (#8897)
7c9f60169 exit=0   fix(hir): preserve shared cells (#8896)

Validation

scripts/run_lint_gates.shall 53 gates pass (compile tier skipped). Runtime 2759/0 under RUST_TEST_THREADS=1. Census exits 0, and its self-test sabotage is non-vacuous again.

How this got merged

I audited #8899 by running the five gates that looked topically relevant to a GC diff and merged on that. run_lint_gates.sh exists precisely to stop this, derives its list from test.yml at run time, and its header documents the same failure mode taking down five gates in one day on 2026-08-17. I should have run it; I now do.

Summary by CodeRabbit

  • Bug Fixes

    • Restored carrier detection logic required for shape census validation.
    • Fixed the census self-test and lint checks so string-pinning operates correctly.
    • Preserved existing memoization behavior while improving validation reliability.
  • Documentation

    • Added a changelog entry explaining the required literal condition and validation safeguards.

…anner

#8899 lifted `descriptor.old_carrier || descriptor.cache_carrier` into a
`let is_carrier` binding for its memo key. Semantically inert, but
`shape_descriptor_census.py` pins the whole two-armed expression on purpose,
so `lint` has been red on main since it landed — and the census's own
self-test, which sabotages that exact literal via a no-op-on-absent
`str.replace`, was disarmed at the same time.
@proggeramlug
proggeramlug merged commit 153d5a3 into main Aug 28, 2026
18 of 20 checks passed
@proggeramlug
proggeramlug deleted the fix/8899-census-carrier-pin branch August 28, 2026 05:48
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: de40bde3-ebf2-4aeb-86ff-f7ba4e56f654

📥 Commits

Reviewing files that changed from the base of the PR and between f989075 and 835273d.

📒 Files selected for processing (2)
  • changelog.d/8917-census-carrier-pin.md
  • crates/perry-runtime/src/object/shapes.rs

📝 Walkthrough

Walkthrough

The change restores the literal carrier predicate in scan_shape_table_rekey_mut so the census self-test can match it. The existing is_carrier binding remains the memo key. A changelog entry records the compatibility requirement.

Changes

Carrier census pin

Layer / File(s) Summary
Restore literal carrier predicate
crates/perry-runtime/src/object/shapes.rs, changelog.d/8917-census-carrier-pin.md
scan_shape_table_rekey_mut uses the inline carrier predicate for slot visitation while retaining is_carrier for memoization. The changelog records the census-script requirement.

Estimated code review effort: 1 (Trivial) | ~5 minutes

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/8899-census-carrier-pin

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

proggeramlug added a commit that referenced this pull request Aug 28, 2026
…ber (#8919)

It landed as #8918, not #8917, and #8917 is a different open PR — fragment
filenames are PR-keyed precisely so in-flight PRs cannot collide.

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug pushed a commit to proggeramlug/perry that referenced this pull request Aug 28, 2026
…plit the file

- `lookup` collided with `buffer/view.rs::lookup` in the root-holder gate's
  loose IDENT graph, which is module-blind past depth 0, so view.rs's body text
  (containing VIEW_REGISTRY) counted as reachable and the holder flipped to
  COVERED. Renamed to `lookup_ways`.
- `test_clear_shape_table` dropped every descriptor box while invalidating
  nothing, leaving dangling ways for any clear-then-lookup in tests.
- Extracted the cfg(test) helpers to a sibling file (2057 -> 1933 lines).
- Re-applied PerryTS#8918's census-pinned carrier literal, which this branch's merge
  had reverted.
proggeramlug added a commit that referenced this pull request Aug 28, 2026
…ble (#8917)

* perf(shapes): direct-mapped cache in front of the shape-descriptor table

shape_descriptor_by_id is on the hot property path — it and
shape_descriptor_ensure_with_generation are ~13% of main-thread samples
between them on a dynamic property loop — and every call paid a TLS
fetch, a RefCell borrow and a hash probe to reach a record whose address
never moves. Box<ShapeDescriptor> is stable across rehash, so a 256-way
direct-mapped cache holds the record's address and a hit is mask,
compare, deref. 4 KiB per thread, fixed.

Caches the ADDRESS, not a copy: records are mutated in place
(old_carrier, cache_carrier, keys after evacuation), so a cached copy
would go quietly stale.

Epoch invalidation is selective. Removal frees the box, and one insert
path can replace a live id with a fresh box; both bump. A fresh-id insert
deliberately does not — it cannot invalidate an existing way, and bumping
there would flush the cache on every shape creation.

Measured (idle host, min of 7): overwrite-only 1088ms -> 955ms, -12.2%;
delete-heavy 1222ms -> 1187ms. Short of the 13% of samples because only
by_id is served; ensure_with_generation still probes. Perry remains ~50x
node on this loop — the rest is js_array_get_f64 (324) and
try_read_tracked_gc_header (307), untouched here.

Invalidation test sabotage-checked: a stale way is a dangling pointer to
a dropped box, not a wrong answer.

Suite 2759 passed.

* fix(shapes): unblock the lookup cache — rename the colliding ident, split the file

- `lookup` collided with `buffer/view.rs::lookup` in the root-holder gate's
  loose IDENT graph, which is module-blind past depth 0, so view.rs's body text
  (containing VIEW_REGISTRY) counted as reachable and the holder flipped to
  COVERED. Renamed to `lookup_ways`.
- `test_clear_shape_table` dropped every descriptor box while invalidating
  nothing, leaving dangling ways for any clear-then-lookup in tests.
- Extracted the cfg(test) helpers to a sibling file (2057 -> 1933 lines).
- Re-applied #8918's census-pinned carrier literal, which this branch's merge
  had reverted.

---------

Co-authored-by: Ralph Küpper <ralph3@skelpo.com>
Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant